-
Notifications
You must be signed in to change notification settings - Fork 26.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix parallel catch-all route normalization #59791
fix parallel catch-all route normalization #59791
Conversation
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
Tests Passed |
Stats from current PRDefault BuildGeneral Overall increase
|
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
buildDuration | 12.7s | 12.7s | N/A |
buildDurationCached | 7.2s | 6.2s | N/A |
nodeModulesSize | 202 MB | 202 MB | |
nextStartRea..uration (ms) | 422ms | 425ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
193.HASH.js gzip | 181 B | 182 B | N/A |
3f784ff6-HASH.js gzip | 53.3 kB | 53.3 kB | ✓ |
433-HASH.js gzip | 28.2 kB | 28.3 kB | N/A |
framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
main-app-HASH.js gzip | 239 B | 242 B | N/A |
main-HASH.js gzip | 31.7 kB | 31.7 kB | N/A |
webpack-HASH.js gzip | 1.7 kB | 1.7 kB | N/A |
Overall change | 98.5 kB | 98.5 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
_app-HASH.js gzip | 194 B | 195 B | N/A |
_error-HASH.js gzip | 183 B | 181 B | N/A |
amp-HASH.js gzip | 504 B | 502 B | N/A |
css-HASH.js gzip | 321 B | 321 B | ✓ |
dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | N/A |
edge-ssr-HASH.js gzip | 255 B | 253 B | N/A |
head-HASH.js gzip | 350 B | 349 B | N/A |
hooks-HASH.js gzip | 369 B | 369 B | ✓ |
image-HASH.js gzip | 4.28 kB | 4.28 kB | N/A |
index-HASH.js gzip | 255 B | 256 B | N/A |
link-HASH.js gzip | 2.61 kB | 2.61 kB | ✓ |
routerDirect..HASH.js gzip | 312 B | 311 B | N/A |
script-HASH.js gzip | 385 B | 383 B | N/A |
withRouter-HASH.js gzip | 307 B | 308 B | N/A |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 3.4 kB | 3.4 kB | ✓ |
Client Build Manifests
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
_buildManifest.js gzip | 483 B | 484 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
index.html gzip | 529 B | 526 B | N/A |
link.html gzip | 541 B | 539 B | N/A |
withRouter.html gzip | 525 B | 522 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Edge SSR bundle Size
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
edge-ssr.js gzip | 93.7 kB | 93.7 kB | N/A |
page.js gzip | 147 kB | 147 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 624 B | 626 B | N/A |
middleware-r..fest.js gzip | 151 B | 151 B | ✓ |
middleware.js gzip | 37.4 kB | 37.4 kB | N/A |
edge-runtime..pack.js gzip | 1.92 kB | 1.92 kB | ✓ |
Overall change | 2.07 kB | 2.07 kB | ✓ |
Next Runtimes
vercel/next.js canary | vercel/next.js 12-19-fix_parallel_catch-all_route_normalization | Change | |
---|---|---|---|
app-page-exp...dev.js gzip | 168 kB | 168 kB | ✓ |
app-page-exp..prod.js gzip | 94.2 kB | 94.2 kB | ✓ |
app-page-tur..prod.js gzip | 94.9 kB | 94.9 kB | ✓ |
app-page-tur..prod.js gzip | 89.4 kB | 89.4 kB | ✓ |
app-page.run...dev.js gzip | 138 kB | 138 kB | ✓ |
app-page.run..prod.js gzip | 88.7 kB | 88.7 kB | ✓ |
app-route-ex...dev.js gzip | 24.1 kB | 24.1 kB | ✓ |
app-route-ex..prod.js gzip | 16.7 kB | 16.7 kB | ✓ |
app-route-tu..prod.js gzip | 16.7 kB | 16.7 kB | ✓ |
app-route-tu..prod.js gzip | 16.3 kB | 16.3 kB | ✓ |
app-route.ru...dev.js gzip | 23.5 kB | 23.5 kB | ✓ |
app-route.ru..prod.js gzip | 16.3 kB | 16.3 kB | ✓ |
pages-api-tu..prod.js gzip | 9.37 kB | 9.37 kB | ✓ |
pages-api.ru...dev.js gzip | 9.64 kB | 9.64 kB | ✓ |
pages-api.ru..prod.js gzip | 9.37 kB | 9.37 kB | ✓ |
pages-turbo...prod.js gzip | 21.9 kB | 21.9 kB | ✓ |
pages.runtim...dev.js gzip | 22.5 kB | 22.5 kB | ✓ |
pages.runtim..prod.js gzip | 21.9 kB | 21.9 kB | ✓ |
server.runti..prod.js gzip | 49.4 kB | 49.4 kB | N/A |
Overall change | 882 kB | 882 kB | ✓ |
Diff details
Diff for server.runtime.prod.js
Diff too large to display
a1f352d
to
e609c1b
Compare
appPaths = Object.fromEntries( | ||
Object.entries(appPaths).map(([k, v]) => [k, v.sort()]) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noticed this was in build
but not dev
, so copied from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaning into that abstraction, is there some place where we could have added it only once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, need to think about this some more, but it's definitely a bit strange that dev-app-page-route-matcher-provider
duplicates this logic but the other matcher providers don't. Seems like this needs to be happening somewhere higher in the stack.
@@ -187,38 +186,6 @@ createNextDescribe( | |||
expect(pageText).toContain('parallel/(new)/@baz/nested/page') | |||
}) | |||
|
|||
it('should throw an error when a route groups causes a conflict with a parallel segment', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test was reworked and moved here
In it's place I kept the existing implementation but verified that the page slot took precedence over the parallel page.
e609c1b
to
408e7eb
Compare
408e7eb
to
44ab019
Compare
try { | ||
process.env.NEXT_MINIMAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this fixing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I'm no longer throwing a build error if there are two matching children
parallel segments, appPaths will technically contain 2 pages and will throw an error when attempting to require
it because the FlightManifestPlugin
will have only generated a manifest for a single entrypoint. Since the other entry won't be used it seems safe to ignore, and loadClientReferenceManifest
could potentially return undefined so I just lifted the try a bit higher.
appPaths = Object.fromEntries( | ||
Object.entries(appPaths).map(([k, v]) => [k, v.sort()]) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaning into that abstraction, is there some place where we could have added it only once?
@@ -0,0 +1,99 @@ | |||
import { normalizeCatchAllRoutes } from './normalize-catchall-routes' | |||
|
|||
describe('normalizeCatchallRoutes', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful
What?
Catch-all routes are being matched to parallel routes which causes issues like an interception route being handled by the wrong page component, or a page component being associated with multiple pages resulting in a "You cannot have two parallel pages that resolve to the same path" build error.
Why?
#58215 fixed a bug that caused catchall paths to not properly match when used with parallel routes. In other words, a catchall slot wouldn't render on a page that could match that catch all. Or a catchall page wouldn't match a slot. At build time, a normalization step was introduced to take application paths and attempt to perform this matching behavior.
However in it's current form, this causes the errors mentioned above to manifest. To better illustrate the problem, here are a few examples:
Given:
The normalization logic would produce:
The interception route will now be improperly handled by
[...slug]/page
rather than the interception handler.Another example, which rather than incorrectly handling a match, will produce a build error:
Given:
The normalization logic would produce:
The parallel catch-all slot is now part of
/
. This means when building the loader tree, twochildren
parallel segments (aka page components) will be found when hitting/
, which is an error.The error that was added here was originally intended to help catch scenarios like:
/app/(group-a)/page
and/app/(group-b)/page
. However it also throws for parallel slots, which isn't necessarily an error (especially since the normalization logic will push potential matches).How?
There are two small fixes in this PR, the rest are an abundance of e2e tests to help prevent regressions.
appPaths
.next-app-loader
, we check to see if it's because we already matched a page component but we also detected a parallel slot that would have matched the page slot. In this case, we don't error, since the app can recover from this.loadClientReferenceManifest
is already potentially returning undefined, so this case should already be handled gracefullySeparately, we'll need to follow-up on the Turbopack side to:
Fixes #58272
Fixes #58660
Fixes #58312
Fixes #59782
Fixes #59784
Closes NEXT-1809